feat(dailyhoroscope): fix moon phase/sign on major phase event days#417
feat(dailyhoroscope): fix moon phase/sign on major phase event days#417tavdog merged 1 commit intotronbyt:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an inaccuracy in the daily horoscope applet where moon phase and sign were incorrectly displayed on days coinciding with major moon phase events. The previous method relied on an external source that reported values for the exact event moment, leading to discrepancies when the moon changed signs later in the day. The changes introduce a robust, local calculation method for these specific days, ensuring the displayed information accurately reflects the moon's state at noon UTC. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces complex astronomical calculations to correct moon phase and sign data on major phase event days. My review focuses on improving the new code's readability and maintainability. I have suggested refactoring large data constants into a separate file and simplifying some of the new helper functions for clarity and efficiency.
| _ZODIAC_SIGNS = [ | ||
| "aries", | ||
| "taurus", | ||
| "gemini", | ||
| "cancer", | ||
| "leo", | ||
| "virgo", | ||
| "libra", | ||
| "scorpio", | ||
| "sagittarius", | ||
| "capricorn", | ||
| "aquarius", | ||
| "pisces", | ||
| ] | ||
|
|
||
| # Periodic terms for the moon longitude (Sigma-l) (Meeus Table 47.A) | ||
| # Each row: [D, M, M', F, coefficient (millionths of a degree)] | ||
| _MOON_LON_TERMS = [ | ||
| [0, 0, 1, 0, 6288774], | ||
| [2, 0, -1, 0, 1274027], | ||
| [2, 0, 0, 0, 658314], | ||
| [0, 0, 2, 0, 213618], | ||
| [0, 1, 0, 0, -185116], | ||
| [0, 0, 0, 2, -114332], | ||
| [2, 0, -2, 0, 58793], | ||
| [2, -1, -1, 0, 57066], | ||
| [2, 0, 1, 0, 53322], | ||
| [2, -1, 0, 0, 45758], | ||
| [0, 1, -1, 0, -40923], | ||
| [1, 0, 0, 0, -34720], | ||
| [0, 1, 1, 0, -30383], | ||
| [2, 0, 0, -2, 15327], | ||
| [0, 0, 1, 2, -12528], | ||
| [0, 0, 1, -2, 10980], | ||
| [4, 0, -1, 0, 10675], | ||
| [0, 0, 3, 0, 10034], | ||
| [4, 0, -2, 0, 8548], | ||
| [2, 1, -1, 0, -7888], | ||
| [2, 1, 0, 0, -6766], | ||
| [1, 0, -1, 0, -5163], | ||
| [1, 1, 0, 0, 4987], | ||
| [2, -1, 1, 0, 4036], | ||
| [2, 0, 2, 0, 3994], | ||
| [4, 0, 0, 0, 3861], | ||
| [2, 0, -3, 0, 3665], | ||
| [0, 1, -2, 0, -2689], | ||
| [2, 0, -1, 2, -2602], | ||
| [2, -1, -2, 0, 2390], | ||
| [1, 0, 1, 0, -2348], | ||
| [2, -2, 0, 0, 2236], | ||
| [0, 1, 2, 0, -2120], | ||
| [0, 2, 0, 0, -2069], | ||
| [2, -2, -1, 0, 2048], | ||
| [2, 0, 1, -2, -1773], | ||
| [2, 0, 0, 2, -1595], | ||
| [4, -1, -1, 0, 1215], | ||
| [0, 0, 2, 2, -1110], | ||
| [3, 0, -1, 0, -892], | ||
| [2, 1, 1, 0, -810], | ||
| [4, -1, -2, 0, 759], | ||
| [0, 2, -1, 0, -713], | ||
| [2, 2, -1, 0, -700], | ||
| [2, 1, -2, 0, 691], | ||
| [2, -1, 0, -2, 596], | ||
| [4, 0, 1, 0, 549], | ||
| [0, 0, 4, 0, 537], | ||
| [4, -1, 0, 0, 520], | ||
| [1, 0, -2, 0, -487], | ||
| [2, 1, 0, -2, -399], | ||
| [0, 0, 2, -2, -381], | ||
| [1, 1, 1, 0, 351], | ||
| [3, 0, -2, 0, -340], | ||
| [4, 0, -3, 0, 330], | ||
| [2, -1, 2, 0, 327], | ||
| [0, 2, 1, 0, -323], | ||
| [1, 1, -1, 0, 299], | ||
| [2, 0, 3, 0, 294], | ||
| ] |
There was a problem hiding this comment.
To improve readability and maintainability, move the _ZODIAC_SIGNS and _MOON_LON_TERMS constants to a separate file (e.g., lunar_data.star) and load them. This aligns with the repository's general rule to keep large data structures out of the main application logic.
References
- To improve readability and maintainability, move large data structures into a separate file and load them into the main application logic.
| if d < 0: | ||
| d = d + 360.0 |
There was a problem hiding this comment.
This if block is redundant. In Starlark, the modulo operator % with a positive divisor ensures a non-negative result. d % 360.0 will always be in the range [0.0, 360.0), so this check can be removed.
| year = int(t.format("2006")) | ||
| month = int(t.format("1")) | ||
| day = int(t.format("2")) | ||
| hour = int(t.format("15")) | ||
| minute = int(t.format("4")) | ||
| second = int(t.format("5")) |
There was a problem hiding this comment.
Instead of formatting the time object to a string and parsing components, access them directly via attributes like t.year, t.month, etc. This is more direct and readable.
| year = int(t.format("2006")) | |
| month = int(t.format("1")) | |
| day = int(t.format("2")) | |
| hour = int(t.format("15")) | |
| minute = int(t.format("4")) | |
| second = int(t.format("5")) | |
| year = t.year | |
| month = t.month | |
| day = t.day | |
| hour = t.hour | |
| minute = t.minute | |
| second = t.second |
|
I'm going to keep the code as is (and not adopt the Gemini suggestions) unless reviewers think otherwise :) |
2 participants
Astro-seek reports moon phase and sign for the exact event moment on New Moon/Full Moon/quarter days, not the calendar day. This caused wrong sign/phase display when the Moon changed signs after the event.
Fix: detect event timestamps, compute sign locally (using Meeus Ch 47 algorithms) and determine phase relative to noon UTC. Non-event days unchanged.